-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Knowage 8043 #891
Knowage 8043 #891
Conversation
This reverts commit 6f40ce8.
Files changed (all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are required allover modified files
try { | ||
PathTraversalChecker.get(fileName, targetDirectory.getName()); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error managing preview file for file: " + fileName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathTraversalChecker should throw only PathTraversalAttackException exception, that is runtime exception, so we can remove the try catch bloch
PathTraversalChecker.get(fileName, targetDirectory.getName()); | ||
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error managing preview file for file: " + fileName); | ||
} | ||
FileUtils.checkPathTraversalAttack(fileName, targetDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this method and use PathTraversalChecker instead
|
||
java.nio.file.Path outputPath = Paths.get(SpagoBIUtilities.getResourcePath(), "dossier", String.valueOf(documentId), fileName); | ||
File file = outputPath.toFile(); | ||
String outPath = SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator + fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use PathTraversalChecker.get method:
PathTraversalChecker.get(SpagoBIUtilities.getResourcePath() + separator + "dossier" , documentId, fileName)
throw new PathTraversalAttackException( | ||
"Error getting removind/moving dataset file for orginal file " + originalFileName + " and new file " + newFileName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this part
@@ -37,6 +37,25 @@ private PathTraversalChecker() { | |||
throw new IllegalStateException("This class provides utility methods. It cannot be instantiated"); | |||
} | |||
|
|||
public static void get(String firstDirectory, String... otherFolders) throws PathTraversalAttackException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename firstDirectory as "trustedDirectory" or "safeDirectory"
} catch (Exception e) { | ||
logger.error(e); | ||
throw new PathTraversalAttackException(e.getMessage()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code is not throwing any exception (apart from runtime PathTraversalAttackException), we can remove the try catch block
} catch (Exception e) { | ||
throw new PathTraversalAttackException("Error deleting dataset file " + fileName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this catch block
Requested pull changes done. (Removed try/catch for PathTraversalChecker methods, renamed PathTraversalChecker.get property name.
@@ -107,7 +108,7 @@ public void doService() { | |||
// checks for path traversal attacks | |||
private void checkRequiredFile(String fileName) { | |||
File targetDirectory = GeneralUtilities.getPreviewFilesStorageDirectoryPath(); | |||
FileUtils.checkPathTraversalAttack(fileName, targetDirectory); | |||
PathTraversalChecker.get(fileName, targetDirectory.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't make sense: inputs are reversed, since targetDirectory is the safe dir and filename is what has to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
JSONObject response = new JSONObject(); | ||
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | ||
PathTraversalChecker.get(fileName, dossierDir.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputs reversed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -171,8 +169,7 @@ public Response checkPathFile(@QueryParam("templateName") String fileName, @Quer | |||
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | |||
JSONObject response = new JSONObject(); | |||
try { | |||
PathTraversalChecker.isValidFileName(fileName); | |||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | |||
PathTraversalChecker.get(fileName, dossierDir.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputs reversed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase()); | ||
if (originalDatasetFile.exists()) { | ||
|
||
if (newDatasetFile != null && originalDatasetFile != null && originalDatasetFile.exists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how files can be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
File datasetFile = new File(filePath + fileName); | ||
if (datasetFile.exists()) { | ||
|
||
if (datasetFile != null && datasetFile.exists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how files can be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
PathTraversalChecker.isValidFileName(fileName); | ||
File attachment = new File(fileName); | ||
|
||
if (attachment != null && attachment.exists() && attachment.isFile()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how files can be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
File attachment = new File(dh.getName()); | ||
if (attachment.exists() && attachment.isFile()) { | ||
String fileName = dh.getName(); | ||
PathTraversalChecker.isValidFileName(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fileName seems to be a complete path instead of a single file name. Please check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to talk about this in today's call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert the changes, most likely this is a false positive
logger.error("File " + imageFile.getPath() + " not found"); | ||
return; | ||
} | ||
PathTraversalChecker.get(parent.toString(), fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent is NOT a safe dir, imageDirectory or imageTmpDir are safe.
Filename is a complete path, don't see how PathTraversalChecker.get will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to talk about this in today's call.
Added required pull changes
PathTraversalChecker.get(safeDirectory, fileName); | ||
String outPath = safeDirectory + separator + fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge them in 1 line: PathTraversalChecker.get has to return a File or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
ResponseBuilder responseBuilder = null; | ||
|
||
File file = new File(outPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge this with the above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
PathTraversalChecker.get(filePath, originalFileName); | ||
File originalDatasetFile = new File(filePath + originalFileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge those lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
PathTraversalChecker.get(fileNewPath, newFileName + "." + fileType.toLowerCase()); | ||
File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge those lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
File attachment = new File(dh.getName()); | ||
if (attachment.exists() && attachment.isFile()) { | ||
String fileName = dh.getName(); | ||
PathTraversalChecker.isValidFileName(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert the changes, most likely this is a false positive
PathTraversalChecker.get(fileName, directory); | ||
htmlFile = new File(directory, fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge those 2 lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
File htmlFile = PathTraversalChecker.get(BirtReportServlet.OUTPUT_FOLDER, reportExecutionId, fileName);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
PathTraversalChecker.get(rootDir.getName(), fileName); | ||
File workDir = new File(rootDir, fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge those 2 lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Changed preventPathTraversalAttack method to private, fixed in other files except DossierActivityResource.
Updated PathTraversalChecker.get
Updated PathTraversalChecker.get inside importTemplateFile method
Fixed htmlFile = PathTraversalChecker.get
Chages reverted, false positive
PathTraversal fixes
Added docs
Changes with Davide Z. (Meet)
knowageutils/src/test/java/it/eng/spagobi/security/utils/PasswordEncryptionToolController.java
Outdated
Show resolved
Hide resolved
knowageutils/src/main/java/it/eng/spagobi/utilities/SpagoBIAccessUtils.java
Outdated
Show resolved
Hide resolved
...rc/main/java/it/eng/spagobi/tools/dataset/utils/datamart/DefaultEngineDatamartRetriever.java
Outdated
Show resolved
Hide resolved
knowagetalendengine/src/main/java/it/eng/spagobi/engines/talend/runtime/RuntimeRepository.java
Outdated
Show resolved
Hide resolved
knowagebirtreportengine/src/main/java/it/eng/spagobi/engines/birt/BirtImageServlet.java
Outdated
Show resolved
Hide resolved
knowage-core/src/main/java/it/eng/spagobi/api/DossierActivityResource.java
Outdated
Show resolved
Hide resolved
knowage-core/src/main/java/it/eng/spagobi/api/DocumentExecutionResource.java
Show resolved
Hide resolved
...jasperreportengine/src/main/java/it/eng/spagobi/engines/jasperreport/JasperReportRunner.java
Outdated
Show resolved
Hide resolved
Changes from the meet
Files changed: